fix: the guard classifies an arithmetic shift correctly, not as a heredoc (iss-184) - #199
Merged
Merged
Conversation
iss-184 (critical, guard heredoc arithmetic-shift bypass), iss-185 (critical, scanner adjacent-secret boundary bypass), iss-186 (minor, capture transition remove-failure strands an issue id), iss-187 (minor, rules.Merge panics on a nil-Domains base). Each carries a reproducing test verified independently before capture. This PR fixes iss-184; the other three remain open for a future round. Assisted-by: Claude:claude-opus-5
…iss-184) An unquoted arithmetic left shift with an identifier operand ($((1<<shift))) was misparsed as a here-document delimiter, and skipHeredocBodies then silently swallowed every remaining line of the command looking for a terminator that never appears -- dropping a later git push --force or rm -rf from ever reaching command position with no error and no signal. A genuinely unterminated heredoc had the same gap. skipHeredocBodies now reports whether it actually found each pending heredoc's terminator; tokenize turns a miss into ErrUnparsableCommand, the same class an unterminated quote already uses, so the guard fails open LOUDLY per its own documented contract instead of silently. Detectors: TestArithmeticShiftByIdentifierIsRejected and TestTokenizeRejectsUnterminatedHeredoc, both watched failing on pre-fix code for the claimed reason and passing after. An existing test that pinned the old silent-swallow as intended behaviour is updated to assert the corrected contract. Assisted-by: Claude:claude-opus-5
Moves iss-184 open -> resolved via abcd capture resolve (impact: fix). DECISIONS.md gets one dated round-summary line for the bug-hunt loop's first round, naming the fix and the three bugs captured but not fixed this round (iss-185, iss-186, iss-187). CHANGELOG gains the user-facing Fixed entry. Assisted-by: Claude:claude-opus-5
The pre-PR security review found that the first pass only closed the unterminated half of the heredoc-vs-arithmetic-shift confusion: an attacker who supplies a later line matching the misread "delimiter" (e.g. appending a bare `shift`) still finds a coincidental terminator, so the guarded command is still silently swallowed with no error. Root cause: a real heredoc delimiter word is never immediately followed by a bare `(` or `)` with no separator -- its body and terminator line have to come first. `$((expr<<ident))` always produces that shape (readHeredocDelim stops at the arithmetic expression's own closing paren), so `<<` there is now classified correctly at tokenize time -- same as the existing literal-digit case -- and the rest of the command is checked normally regardless of what any later line contains. This is the actual fix; the previous commit's unterminated-body-to-error path is kept as defense in depth for genuinely malformed heredocs, a distinct, narrower gap. Detectors: TestArithmeticShiftByIdentifierIsNotAHeredoc and TestArithmeticShiftCoincidentalDelimiterStillBlocks (the exact adversarial payload from the security review), both watched failing against the previous commit's code and passing after this one. Assisted-by: Claude:claude-opus-5
The ledger resolution, DECISIONS.md round line, and CHANGELOG entry described the first pass's error-on-unterminated-body mitigation as a root-cause fix. It wasn't -- the pre-PR security review demonstrated a live bypass surviving it. All three now describe the two-pass fix accurately: the classification fix that actually closes the hole, and the unterminated-body fix kept as defense in depth for a distinct, narrower gap. Assisted-by: Claude:claude-opus-5
This was referenced Aug 5, 2026
…heredoc-arithmetic-shift-bypass # Conflicts: # .abcd/work/DECISIONS.md
REPPL
added a commit
that referenced
this pull request
Aug 6, 2026
… (iss-118) Both files are append-only ledgers of anonymous, dated, order-independent entries that never need identity -- the same shape CHANGELOG.md already carries merge=union for. A concurrent append to either has nothing to actually conflict over, so the union driver keeps both sides instead of stopping the merge, the way it already does for CHANGELOG.md. Prompted directly by the bug-hunt loop's bugfix/iss-184-... branch hitting this exact conflict on PR #199 against DECISIONS.md -- a collision iss-118 had already diagnosed (filed after an earlier merge hit the same wall) but left unresolved pending this design call. Full atomicisation (per-decision records, a new id family, an armed uniqueness detector) was the other option iss-118 named; not pursued here as disproportionate to a minor, low-traffic hotspot. Assisted-by: Claude:claude-sonnet-5
REPPL
added a commit
that referenced
this pull request
Aug 6, 2026
…metic-shift-bypass fix: the guard classifies an arithmetic shift correctly, not as a heredoc (iss-184)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes iss-184 (critical), one of four bugs surfaced by this round's multi-angle bug-hunt sweep and independently verified with failing tests. The other three (iss-185, iss-186, iss-187) are captured to the ledger but not fixed here.
The bug
internal/core/guard/tokenize.go's<<handling already special-cased the literal-digit arithmetic-shift form ($((1<<20))is not a heredoc —isDelimStartrejects a delimiter word starting with a digit). ButisDelimStartaccepts any identifier-shaped word, so$((1<<shift))— an identifier operand — still read as a heredoc delimiter.skipHeredocBodies(tokenize.go:264) then scanned for a line equal to the bogus delimiter and, whether or not one existed, consumed every line up to it as unchecked body text — dropping a later dangerous command (git push --force,rm -rf, etc.) from ever reaching command position.Registry.CheckreturnedVerdictAllowwith no error and no signal: a silent guard bypass, reachable through both CLI front doors (guard checkandguard hook).How it was reproduced and verified
An independent fresh subagent, blind to the finder, wrote a failing test and confirmed it failed on
mainfor the claimed reason (not a compile error or unrelated failure):What changed and why
Two commits, because the first pass under-fixed it and a pre-PR adversarial security review (mandatory here — this touches the guard trust boundary) caught it:
skipHeredocBodiesreport whether it actually found each pending heredoc's terminator, turning a miss intoErrUnparsableCommand(the guard's existing fail-open-loud contract). This closed the case where no matching line exists — but the security review constructed the exploit it missed: an attacker who appends a line that happens to equal the misread "delimiter" (e.g. a bareshift, a harmless no-op) still gets a match, so the swallow still succeeds silently. The independent correctness review, reviewing the same commit separately, converged on the identical finding.(or)with no separator — its body and terminator line have to come first.$((expr<<ident))always produces exactly that shape (readHeredocDelimstops at the arithmetic expression's own closing paren), so<<in that position is now classified correctly at tokenize time, the same way the literal-digit case already was. The rest of the command is read and matched normally, regardless of what any later line contains. The first pass's unterminated-body-to-error path is kept as defense in depth for a distinct, narrower gap: a genuinely malformed heredoc (cat <<EOFwith no closingEOFline) previously also silently swallowed the rest of the input with no signal at all.Evidence
internal/core/guard/tokenize.go:156-181— the<<classification, before and after.internal/core/guard/tokenize.go:264-296—skipHeredocBodies's terminator-found signal.internal/core/guard/tokenize_test.go—TestArithmeticShiftByIdentifierIsNotAHeredoc,TestArithmeticShiftCoincidentalDelimiterStillBlocks(the exact adversarial payload from the security review),TestTokenizeRejectsUnterminatedHeredoc— each watched failing against the pre-fix code for the claimed reason, and passing after.internal/surface/cli/guard.go:80-124— confirmsErrUnparsableCommandsurfaces visibly through bothguard check(exit 2, printed message) andguard hook(failOpen, exit 1,NOT CHECKED … runs UNGUARDEDon stderr) rather than being swallowed.Ledger
impact: fix) viaabcd capture resolve..abcd/work/DECISIONS.mdgains one dated round-summary line;CHANGELOG.mdgains a### Fixedentry under[Unreleased].Gates
make preflightexit 0 (build, vet,go test ./...,go test -race ./internal/...);gofmt -l .clean;go run ./cmd/record-lintexit 0, 0 blockers (pre-existing WARNs only, baseline unchanged). Pre-PR reviews: one adversarial correctness review and one adversarial security review (required — this PR touches the guard trust boundary), each in its own fresh subagent. Both independently found the first pass's gap; both findings are fixed in the second commit, with regression tests reproducing the exact adversarial payload the security review demonstrated.